Fix failing default transform for `LKJCorr` by juanitorduz · Pull Request #7065 · pymc-devs/pymc

juanitorduz · 2023-12-13T09:40:03Z

Wt take a different direction from #7023

📚 Documentation preview 📚: https://pymc--7065.org.readthedocs.build/en/7065/

codecov · 2023-12-13T09:48:46Z

Codecov Report

Merging #7065 (67b80c6) into main (2e05854) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7065   +/-   ##
=======================================
  Coverage   92.19%   92.20%           
=======================================
  Files         101      101           
  Lines       16893    16901    +8     
=======================================
+ Hits        15575    15584    +9     
+ Misses       1318     1317    -1

Files	Coverage Δ
pymc/distributions/multivariate.py	`93.53% <100.00%> (+0.16%)`	⬆️
pymc/logprob/transform_value.py	`93.75% <100.00%> (ø)`

tests/distributions/test_multivariate.py

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

tests/distributions/test_multivariate.py

ricardoV94 · 2023-12-13T10:33:02Z

Is it good on your end? Asking because it's marked as a draft

juanitorduz · 2023-12-13T10:33:40Z

@ricardoV94 With the new suggestion the test fails locally with the

NotImplementedError: Univariate transform MultivariateIntervalTransform cannot be applied to multivariate lkjcorr_rv{1, (0, 0), floatX, False}

🤔

ricardoV94 · 2023-12-13T10:34:47Z

@ricardoV94 With the new suggestion the test fails with the

NotImplementedError: Univariate transform MultivariateIntervalTransform cannot be applied to multivariate lkjcorr_rv{1, (0, 0), floatX, False}

🤔

Where is that check coming from? We might need to add some meta-info to the Transform

ricardoV94 · 2023-12-13T10:41:43Z

The problem is the logp of the distribution is incorrectly implemented. It's returning a scalar instead of a vector of shape=(3,)

juanitorduz · 2023-12-13T10:46:54Z

Yeah! It is failing locally. It's good that you caught up on this with the test! Do you think there is an "easy" fix?

ricardoV94 · 2023-12-13T10:47:41Z

We should add a NotImplementedError in the logp for when value.ndim > 1, since it's not properly supported AFAICT: #5383

You can parametrize the test to have two cases shape=(2,) and shape=(3,2) the second of which is expected to fail. You can mark specific parametrizations with pytest.param like here:

pymc/tests/distributions/test_multivariate.py

Lines 1048 to 1055 in 9b4bf2a

    
                       pytest.param( 
        
                           np.array([0.1441, 0.1363, 0.1385, 0.1348, 0.1521, 0.1500, 0.1442]), 
        
                           4, 
        
                           None, 
        
                           np.array([1, 1, 1, 1, 0, 0, 0]), 
        
                           marks=pytest.mark.xfail( 
        
                               rises=AssertionError, reason="Known failure in mode approximation " 
        
                           ),

Also this test shouldn't be in the TestLKJCorr class, since that is just for the RandomVariable checks. You can make it a standalone test

ricardoV94 · 2023-12-13T10:51:17Z

Yeah! It is failing locally. It's good that you caught up on this with the test! Do you think there is an "easy" fix?

It's not trivial, it requires thinking carefully about batch dimensions, like we did in this PR: #6897
But it would be a great addition. I think it should be done in a separate PR though

ricardoV94 · 2023-12-13T10:52:59Z

Also could you reintroduce the change from the other PR where we always run this check instead of being in the else branch?

pymc/pymc/logprob/transform_value.py

Lines 128 to 133 in 04a03b5

    
           else: 
        
               # Check there is no broadcasting between logp and jacobian 
        
               if logp.type.broadcastable != log_jac_det.type.broadcastable: 
        
                   raise ValueError( 
        
                       f"The logp of {rv_op} and log_jac_det of {transform} are not allowed to broadcast together. " 
        
                       "There is a bug in the implementation of either one."

juanitorduz · 2023-12-13T11:23:08Z

Added the suggested changes :)

tests/distributions/test_multivariate.py

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

ricardoV94 · 2023-12-13T11:51:00Z

We should add a check in the logp similar to this:

pymc/pymc/distributions/multivariate.py

Lines 1249 to 1252 in e67a317

    
           (value,) = values 
        
           if value.ndim > 1: 
        
               raise ValueError("_LKJCholeskyCov logp is only implemented for vector values (ndim=1)")

Should be a NotImplementedError though

tests/distributions/test_multivariate.py

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

juanitorduz · 2023-12-13T12:50:48Z

We now have two tests failing because of the new NotImplementedError. Should I remove such cases (via xfail)?

tests/distributions/test_multivariate.py::TestMoments::test_lkjcorr_moment[3-1-1-expected2] FAILED [ 87%]
tests/distributions/test_multivariate.py::TestMoments::test_lkjcorr_moment[5-1-size3-expected3] FAILED [ 87%]

ricardoV94 · 2023-12-13T12:57:52Z

We now have two tests failing because of the new NotImplementedError. Should I remove such cases (via xfail)?
tests/distributions/test_multivariate.py::TestMoments::test_lkjcorr_moment[3-1-1-expected2] FAILED [ 87%]
tests/distributions/test_multivariate.py::TestMoments::test_lkjcorr_moment[5-1-size3-expected3] FAILED [ 87%]

Yup

juanitorduz · 2023-12-13T13:53:46Z

@ricardoV94 we are back to 🟢 :)

juanitorduz · 2023-12-13T13:59:30Z

Thank you for all your help @ricardoV94 ❤️

ricardoV94 · 2023-12-13T13:59:30Z

Thanks @juanitorduz

change default transform LKJCOrr

e67a317

juanitorduz marked this pull request as draft December 13, 2023 09:40

add test

301b159

ricardoV94 reviewed Dec 13, 2023

View reviewed changes

tests/distributions/test_multivariate.py Outdated Show resolved Hide resolved

Update tests/distributions/test_multivariate.py

bc0d8f5

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

ricardoV94 reviewed Dec 13, 2023

View reviewed changes

tests/distributions/test_multivariate.py Outdated Show resolved Hide resolved

ricardoV94 added the bug label Dec 13, 2023

ricardoV94 changed the title ~~Change default transform LKJCOrr~~ Fix failing default transform for LKJCorr Dec 13, 2023

fix import

9b4bf2a

ricardoV94 approved these changes Dec 13, 2023

View reviewed changes

error handling

21948a4

juanitorduz requested a review from ricardoV94 December 13, 2023 11:22

juanitorduz marked this pull request as ready for review December 13, 2023 11:22

ricardoV94 reviewed Dec 13, 2023

View reviewed changes

tests/distributions/test_multivariate.py Outdated Show resolved Hide resolved

Update tests/distributions/test_multivariate.py

8e76b84

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

juanitorduz added 3 commits December 13, 2023 12:51

remove expected shapre argument

6f03ec5

add notimplemented error for logp

aa7810c

make standalone test

bee4144

ricardoV94 reviewed Dec 13, 2023

View reviewed changes

tests/distributions/test_multivariate.py Outdated Show resolved Hide resolved

Update tests/distributions/test_multivariate.py

67954c7

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>

patch test

67b80c6

juanitorduz requested a review from ricardoV94 December 13, 2023 13:53

ricardoV94 merged commit 851c991 into pymc-devs:main Dec 13, 2023

juanitorduz deleted the issue_7002 branch December 13, 2023 13:59

Comments

Conversation

juanitorduz commented Dec 13, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

juanitorduz commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

juanitorduz commented Dec 13, 2023

Uh oh!

ricardoV94 commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

juanitorduz commented Dec 13, 2023

Uh oh!

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

Uh oh!

juanitorduz commented Dec 13, 2023

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

juanitorduz commented Dec 13, 2023

Uh oh!

juanitorduz commented Dec 13, 2023

Uh oh!

ricardoV94 commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juanitorduz commented Dec 13, 2023 •

edited by github-actions bot

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

juanitorduz commented Dec 13, 2023 •

edited

Loading

ricardoV94 commented Dec 13, 2023 •

edited

Loading

ricardoV94 commented Dec 13, 2023 •

edited

Loading